InstCombine: fold(select C, (X | A), X) | B into X | select C, (A | B), B. (#154246) #154267
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
llvm:instcombine
|
@llvm/pr-subscribers-llvm-transforms Author: Rohan A M (badwriter123) Changes#154246 I noticed that the InstCombine was missing a simple factoring opportunity when a select feeds a bitwise-or.
I added a fold in visitOr that recognizes those shapes (including the commuted outer-or) and performs the required rewrite. I have also added a test at llvm/test/Transforms/InstCombine/or-select-factor.ll to cover this transformation. No other files or changes were made. Full diff: https://github.com/llvm/llvm-project/pull/154267.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index 6e46898634070..ac78b0276d594 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -2695,6 +2695,40 @@ Instruction *InstCombinerImpl::visitAnd(BinaryOperator &I) {
if (Instruction *FoldedLogic = foldBinOpIntoSelectOrPhi(I))
return FoldedLogic;
+ // Factor a common operand across select feeding an 'or':
+ // (select Cond, (X | A), X) | B -> X | select Cond, (A | B), B
+ // (select Cond, X, (X | A)) | B -> X | select Cond, B, (A | B)
+ // and commuted variants swapping the operands of the outer 'or'.
+ auto TryFactorSelectOr = [&](Value *SelOp, Value *OtherOp) -> Instruction * {
+ auto *SI = dyn_cast<SelectInst>(SelOp);
+ if (!SI)
+ return nullptr;
+
+ Value *Cond = SI->getCondition();
+ Value *T = SI->getTrueValue();
+ Value *F = SI->getFalseValue();
+ Value *X, *A;
+
+ // Match: select Cond, (X|A), X
+ if (match(T, m_c_Or(m_Value(X), m_Value(A))) && F == X) {
+ Value *AorB = Builder.CreateOr(A, OtherOp);
+ Value *InnerSel = Builder.CreateSelect(Cond, AorB, OtherOp);
+ return BinaryOperator::CreateOr(X, InnerSel);
+ }
+ // Match: select Cond, X, (X|A)
+ if (match(F, m_c_Or(m_Value(X), m_Value(A))) && T == X) {
+ Value *AorB = Builder.CreateOr(A, OtherOp);
+ Value *InnerSel = Builder.CreateSelect(Cond, OtherOp, AorB);
+ return BinaryOperator::CreateOr(X, InnerSel);
+ }
+ return nullptr;
+ };
+
+ if (Instruction *R = TryFactorSelectOr(Op0, Op1))
+ return R;
+ if (Instruction *R = TryFactorSelectOr(Op1, Op0))
+ return R;
+
if (Instruction *DeMorgan = matchDeMorgansLaws(I, *this))
return DeMorgan;
@@ -4022,6 +4056,42 @@ Instruction *InstCombinerImpl::visitOr(BinaryOperator &I) {
}
}
+ // Factor a common operand across select feeding an 'or' before pushing the
+ // constant into the select arms:
+ // (select Cond, (X | A), X) | B -> X | select Cond, (A | B), B
+ // (select Cond, X, (X | A)) | B -> X | select Cond, B, (A | B)
+ // and commuted variants swapping the operands of the outer 'or'.
+ auto TryFactorSelectOrEarly = [&](Value *SelOp,
+ Value *OtherOp) -> Instruction * {
+ auto *SI = dyn_cast<SelectInst>(SelOp);
+ if (!SI)
+ return nullptr;
+
+ Value *Cond = SI->getCondition();
+ Value *T = SI->getTrueValue();
+ Value *F = SI->getFalseValue();
+ Value *X, *A;
+
+ // Match: select Cond, (X|A), X
+ if (match(T, m_c_Or(m_Value(X), m_Value(A))) && F == X) {
+ Value *AorB = Builder.CreateOr(A, OtherOp);
+ Value *InnerSel = Builder.CreateSelect(Cond, AorB, OtherOp);
+ return BinaryOperator::CreateOr(X, InnerSel);
+ }
+ // Match: select Cond, X, (X|A)
+ if (match(F, m_c_Or(m_Value(X), m_Value(A))) && T == X) {
+ Value *AorB = Builder.CreateOr(A, OtherOp);
+ Value *InnerSel = Builder.CreateSelect(Cond, OtherOp, AorB);
+ return BinaryOperator::CreateOr(X, InnerSel);
+ }
+ return nullptr;
+ };
+
+ if (Instruction *R = TryFactorSelectOrEarly(Op0, Op1))
+ return R;
+ if (Instruction *R = TryFactorSelectOrEarly(Op1, Op0))
+ return R;
+
if (Instruction *FoldedLogic = foldBinOpIntoSelectOrPhi(I))
return FoldedLogic;
diff --git a/llvm/test/Transforms/InstCombine/or-select-factor.ll b/llvm/test/Transforms/InstCombine/or-select-factor.ll
new file mode 100644
index 0000000000000..75f62b2679ca0
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/or-select-factor.ll
@@ -0,0 +1,33 @@
+; RUN: opt -passes=instcombine -S < %s | FileCheck %s
+
+; Fold: (select C, (x | a), x) | b -> x | select C, (a | b), b
+
+define i8 @src(i8 %x, i8 %y) {
+; CHECK-LABEL: @src(
+; CHECK-NEXT: [[V0:%.*]] = icmp eq i8 [[Y:%.*]], -1
+; CHECK-NEXT: [[V1:%.*]] = or i8 [[X:%.*]], 4
+; CHECK-NEXT: [[V2:%.*]] = select i1 [[V0]], i8 [[V1]], i8 [[X]]
+; CHECK-NEXT: [[V3:%.*]] = or i8 [[V2]], 1
+; CHECK-NEXT: ret i8 [[V3]]
+;
+ %v0 = icmp eq i8 %y, -1
+ %v1 = or i8 %x, 4
+ %v2 = select i1 %v0, i8 %v1, i8 %x
+ %v3 = or i8 %v2, 1
+ ret i8 %v3
+}
+
+define i8 @tgt(i8 %x, i8 %y) {
+; CHECK-LABEL: @tgt(
+; CHECK-NEXT: [[V0:%.*]] = icmp eq i8 [[Y:%.*]], -1
+; CHECK-NEXT: [[V1:%.*]] = select i1 [[V0]], i8 5, i8 1
+; CHECK-NEXT: [[V2:%.*]] = or i8 [[X:%.*]], [[V1]]
+; CHECK-NEXT: ret i8 [[V2]]
+;
+ %v0 = icmp eq i8 %y, -1
+ %v1 = select i1 %v0, i8 5, i8 1
+ %v2 = or i8 %x, %v1
+ ret i8 %v2
+}
+
+
|
There was a problem hiding this comment.
Miscompilation reproducer: https://alive2.llvm.org/ce/z/_v5Hjz
define i8 @src(i8 %x, i8 noundef %y) {
%v0 = icmp samesign eq i8 %y, -1
%v1 = or i8 %x, 4
%v2 = select i1 %v0, i8 %v1, i8 %x
%v31 = and i8 -2, %v2
ret i8 %v31
}
define i8 @tgt(i8 %x, i8 noundef %y) {
%v31 = or i8 %x, -2
ret i8 %v31
}
Infinite loop reproducer:
; bin/opt -passes=instcombine reduced.ll -S
define i64 @func_48(i1 %cmp268, i64 %conv272) {
entry:
br label %for.body200
for.body200: ; preds = %for.body200, %entry
%0 = phi i64 [ 0, %entry ], [ %or273, %for.body200 ]
%and = and i64 1, %0
%or253 = or i64 %and, 4398243566277173332
%1 = select i1 %cmp268, i64 %or253, i64 %and
%or273 = or i64 %1, %conv272
br label %for.body200
}
| @@ -0,0 +1,33 @@ | |||
| ; RUN: opt -passes=instcombine -S < %s | FileCheck %s | |||
There was a problem hiding this comment.
- Use update_test_checks.py to generate test files.
- Add some negative tests.
- Add some multi-use tests.
- Do not add the tgt function. It should be generated by the script above.
Please read https://llvm.org/docs/InstCombineContributorGuide.html#tests for more information.
| @@ -2695,6 +2695,40 @@ Instruction *InstCombinerImpl::visitAnd(BinaryOperator &I) { | |||
| if (Instruction *FoldedLogic = foldBinOpIntoSelectOrPhi(I)) | |||
| return FoldedLogic; | |||
|
|
|||
| // Factor a common operand across select feeding an 'or': | |||
There was a problem hiding this comment.
Any reason for duplicating the code in visitAnd?
There was a problem hiding this comment.
Hello,
while working on the optimization for the OR operation , I rewrite the expression from (select C, (X | A), X) | B to select C, (A | B),B. The same OR optimization can be used to rewrite AND operations as well. Instead of overengineering , I thought of duplicating it for AND operations.
if not required for AND operations, the code can be discarded.
Fixes #154246
Hello,
I noticed that the InstCombine was missing a simple factoring opportunity when a select feeds a bitwise-or.
For Example in patterns like :
we can pull the X out and rewrite the canonical and optimized form as follows :
I added a fold in visitOr that recognizes those shapes (including the commuted outer-or) and performs the required rewrite.
and also I placed it before the generic "push constant into select arms" fold so we dont lose the structure we need to factor the X cleanly.
I have also added a test at llvm/test/Transforms/InstCombine/or-select-factor.ll to cover this transformation.
No other files or changes were made.